-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add MSI for WSLA #13717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add MSI for WSLA #13717
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks great !
To make this work in the tests, we'll need to:
-
Capture the new MSI in TestGroup.xml
-
Update the install logic in test-setup.ps1
We'll probably going to need a WSLA specific registry key (like what we do for WSL in Computer\HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Lxss\MSI) where the store the MSI product code and installed version (so we can uninstall it easily in scripts)
|
|
||
| $exitCode = (Start-Process -Wait "msiexec.exe" -ArgumentList $MSIArguments -NoNewWindow -PassThru).ExitCode | ||
| if ($exitCode -Ne 0) | ||
| if ($exitCode -NotIn (0, 1605)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for this change ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's because the script currently tries to uninstall the WSL MSI package. We should update that once we have a registry key that stores the WSLA MSI product code and uninstall& install both package in this script
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I am not holding things right, but many times I got into the state of running the test-setup.ps1 with one of the MSIs not being installed, so uninstalling it failed.
But now that I think about it, I guess the registry key shouldn't be there in that case because the uninstall should have removed it
|
|
||
| string(REPLACE "/Zi" "" CMAKE_CXX_FLAGS_DEBUG ${CMAKE_CXX_FLAGS_DEBUG}) # make sure /Zi is removed from the debug flags | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /bigobj /W3 /WX /ZH:SHA_256") | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /bigobj /W3 /WX /ZH:SHA_256 /Zm1000") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, does this make the precompiled header build faster ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it makes it any faster. I added it because my computer was running out of memory frequently.
|
|
||
| set_source_files_properties(${OUTPUT_PACKAGE} PROPERTIES GENERATED TRUE) | ||
|
|
||
| if (DEFINED WSL_POST_BUILD_COMMAND) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also set in the wsl.msi target, we might want to have another "post build" command specific to WSLA, so the same command doesn't run twice here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command is basically calling deploy-to-host.ps1, with the additional parameters set here. The additional parameters specify which of the two to install.
.wix.inis mostly extracted from the existing one.package.wix.into match the folder name, to try and make it more clear which.wixfile is for what. (I had originally also named the WSLA aspackage, but that causes a conflict in the output folder. Since they need to have distinct names, it seemed better to have them match the folder structure.)Pending:
CMakeLists.txtand share it for both packages?